-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add some concepts related to exit(3) #17
Conversation
I think we probably want to reach a conclusion here before moving forward with the changes in the proposal? |
sure. let me mark this a draft for now. |
README.md
Outdated
* Threads created by a thread in a thread group using `wasi_thread_spawn` | ||
is added to the thread group. | ||
|
||
* When a thread is terminated, it's removed from the thread group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need a concept of thread group? Why not just say that wasi_thread_spawn
creates new threads and wasi_thread_exit
terminates the current thread?
The thread group concept seems like something fairly specific to linux and/or the world outside of the process.
If we do want to give a name the container of threads why not use the commonly used term "process"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just wanted a name for what wasi proc_exit terminates. the name "process" is fine for me too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i renamed it to process.
README.md
Outdated
### Changes to WASI `proc_exit` | ||
|
||
With this proposal, the `proc_exit` function takes extra responsibility | ||
to terminate all threads in the thread group, not only the calling one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again just not just drop the in the thread group
part of this phrase.
README.md
Outdated
When a thread caused a trap, it terminates all threads in the thread group | ||
similarly to `proc_exit`. | ||
|
||
### Thread group exit status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about Process exit status
? This terminology makes sense to me and aligns with the proc_exit
name (the proc here stands for process
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we end up deciding to add wasi_thread_exit
in #7, I think I have some concerns about this:
- I think talking of "processes" and "thread groups" could be confusing: I see that @sbc100 has asked for changes to the terminology and I don't want to nitpick that, but since Wasm/WASI does not really have these concepts and people already have definitions for them in mind, I would prefer to not use them in the proposal — e.g., we could speak instead of "running module" and "all the threads" instead.
- I think this change, as written, risks over-specifying things at this stage: e.g., I'm not too sure we want to terminate all threads if one of them traps (I assume via Wasm
unreachable
). And why concern ourselves (or not) about which thread determines thewasi_proc_exit
exit code? I would prefer these kinds of thing to be left open-ended until we have a more clear idea if they must be specified.
i want to introduce a term because i feel "all the threads" is a bit vague. as @sbc100 pointed out, we already have the term in the name of "wasi_proc_exit".
everything in this PR was something i actually had to decide while implementing wasi-threads in toywasm. |
The current wasi-threads has no thread-exit functionality. Thus it isn't straightforward to implement pthread_exit without leaking thread context. This commit simply disables pthread_exit for now. Also, instead of abusing `wasi_proc_exit` for thread exit, make let `wasi_thread_start` return. Note: `wasi_proc_exit` is supposed to terminate all threads in the "process", not only the calling thread. Note: Depending on the conclusion of the discussion about `wasi_thread_exit`, we might revisit this change later. References: WebAssembly/wasi-threads#7 WebAssembly/wasi-threads#17
The current wasi-threads has no thread-exit functionality. Thus it isn't straightforward to implement pthread_exit without leaking thread context. This commit simply disables pthread_exit for now. Also, instead of abusing `wasi_proc_exit` for thread exit, make `wasi_thread_start` return. Note: `wasi_proc_exit` is supposed to terminate all threads in the "process", not only the calling thread. Note: Depending on the conclusion of the discussion about `wasi_thread_exit`, we might revisit this change later. References: WebAssembly/wasi-threads#7 WebAssembly/wasi-threads#17
The current wasi-threads has no thread-exit functionality. Thus it isn't straightforward to implement pthread_exit without leaking thread context. This commit simply disables pthread_exit for now. Also, instead of abusing `wasi_proc_exit` for thread exit, make `wasi_thread_start` return. Note: `wasi_proc_exit` is supposed to terminate all threads in the "process", not only the calling thread. Note: Depending on the conclusion of the discussion about `wasi_thread_exit`, we might revisit this change later. References: WebAssembly/wasi-threads#7 WebAssembly/wasi-threads#17
i removed |
* Introduce wasi_thread_exit * Introduce thread group and its exit status
It's a more commen name and matches "proc" in `proc_exit`.
i resolved conflicts after #16 |
README.md
Outdated
of the process. | ||
It's non deterministic which one is chosen. | ||
|
||
If the process gets empty without involving `proc_exit` or a trap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"gets empty" sounds a little odd to me. How about "If the last remaining thread in the process returns from its entry point without trapping or calling proc_exit
it is treated as if proc_exit
was called with exit code 0".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it isn't only about the last thread.
eg. if the second last thread terminated itself with a trap and the last thread returned from wasi_thread_start
, i still want to make the trap represent the process exit status.
also, i want to avoid mentioning the entry point in this sentence because it can involve another topic: #21
how about:
If all the threads in the process have been terminated without calling
`proc_exit` or raising a trap, it's treated as if the last thread called
`proc_exit` with exit code 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thats sounds fine. Perhaps we can define "normal termination" and "early termination" and say "If all the threads in the process terminate normally"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thats sounds fine.
done
Perhaps we can define "normal termination" and "early termination" and say "If all the threads in the process terminate normally"?
i feel it confusing as calling exit() is quite normal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its reasonable to refer to "exit()" as early and/or abnormal termination.
README.md
Outdated
the main thread. | ||
|
||
* Threads created by a thread in a process using `wasi_thread_spawn` | ||
is added to the process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is added to the process. | |
are added to the process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
README.md
Outdated
With this proposal, the `proc_exit` function takes extra responsibility | ||
to terminate all threads in the process, not only the calling one. | ||
|
||
Any of threads in the process can call `proc_exit`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any of threads in the process can call `proc_exit`. | |
Any of the threads in the process can call `proc_exit`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
If all the threads in the process have been terminated without calling | ||
`proc_exit` or raising a trap, it's treated as if the last thread called | ||
`proc_exit` with exit code 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not mention proc_exit
here and just say that exit code is 0
? It might get a bit confusing if e.g. in the future proc_exit
is extended with additional functionality - in that case, this sentence can be interpreted as that additional functionality should also be executed along with returning 0 as exit code (whereas I think it shouldn't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can't imagine any functionalities which proc_exit
might gain but on the other hand should not be a part of this. do you have examples?
even if we avoid mentioning proc_exit
here, when you are adding an extra functionality to proc_exit
, you still need to consider if the change should apply to this sentence or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have examples?
No, it was only hypothetical question.
you still need to consider if the change should apply to this sentence or not.
Yes
|
||
### Process exit status | ||
|
||
If one or more threads call WASI `proc_exit` or raise a trap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it include calling it proc_exit
from the main thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
As is being discussed [elsewhere], either calling `proc_exit` or trapping in any thread should halt execution of all threads. The Wasmtime CLI already has logic for adapting a WebAssembly error code to a code expected in each OS. This change factors out this logic to a new function, `maybe_exit_on_error`, for use within the `wasi-threads` implementation. This will work reasonably well for CLI users of Wasmtime + `wasi-threads`, but embedders will want something better in the future: when a `wasi-threads` threads fails, they may not want their application to exit. Handling this is tricky, because it will require cancelling the threads spawned by the `wasi-threads` implementation, something that is not trivial to do in Rust. With this change, we defer that work until later in order to provide a working implementation of `wasi-threads` for experimentation. [elsewhere]: WebAssembly/wasi-threads#17
### Changes to WASI `proc_exit` | ||
|
||
With this proposal, the `proc_exit` function takes extra responsibility | ||
to terminate all threads in the process, not only the calling one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about simply "When proc_exit
is called the entire process is terminated, including all running threads". I'm not we need the "takes extra responsibility" wording.
It seems fairly clear from its name that proc_exit
would do this, so I don't really see it as "extra responsibility", but maybe thats just my reading of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without wasi-threads, a runtime would simply terminate the calling thread.
when adding wasi-threads support to a runtime, you need to make it terminate other thread too. it would need a significant implementation effort. i feel it's appropriate to call it extra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. But don't think that is clear enough to simply say: "When proc_exit is called the entire process is terminated, including all running threads".
(BTW, the way I see it proc_exit
brings down the entire process (that is that the proc
part means), even without wasi-threads. It just happens that there was only ever one thread previously.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe. i wanted to somehow emphasize that this is something a runtime needs to implement.
README.md
Outdated
|
||
### Traps | ||
|
||
When a thread caused a trap, it terminates all threads in the process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/caused/causes/ ?
How about "When a trap occurs in any thread, the entire process is terminated."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/caused/causes/ ?
probably. i'm not good at english as you may noticed.
How about "When a trap occurs in any thread, the entire process is terminated."?
done.
of the process. | ||
It's non deterministic which one is chosen. | ||
|
||
If all the threads in the process have been terminated without calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about, "If that last running thread in a process terminates without calling proc_exit
or trapping it is treated as if ... "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not only about the last running thread.
consider a process with two threads.
- a thread terminated itself by calling
proc_exit(50)
- another thread, which is
the last running thread
, terminates itself by returning fromwasi_thread_spawn
before the runtime terminates it forproc_exit
. - the exit code of the process should be 50.
i think your wording can be interpreted as the exit code of the process can be 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My my understanding, in your example above proc_exit(50)
causes all threads to be terminated, so its not possible (2) to occor.
Another way of putting it "proc_exitdoes not complete until all threads are terminated, so not thread can exist after
proc_exit` is complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My my understanding, in your example above
proc_exit(50)
causes all threads to be terminated, so its not possible (2) to occor.
if things work in a lockstep, maybe.
but we are talking about threads...
Another way of putting it "proc_exit
does not complete until all threads are terminated, so not thread can exist after
proc_exit` is complete.
in that case, i guess you need to say the thread which called proc_exit
is terminated after all other threads are terminated. i feel it's more complicated than the current sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would odd if we allowed any threads to outlive the thread that calls proc_exit
.. the whole point of that system call is to bring down all the threads so logically nothing else can happen on any thread once that syscall completes.
I don't want to hold up this PR, even though I don't love the way this sentence is phrased. I guess we can land this as it stands and try to improve it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let me know if #27 addresses some of the things you bring up here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % nits
is there any blocker on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to merge this as-is, despite the nits @sbc100 brought up remaining unaddressed. The spec does need to say something about how proc_exit
shuts down all the threads and this starts that process--it's progress! I will immediately open up a follow-up PR to see if I can resolve some of the finer points noted here by others. Thanks @yamt!
Recent changes (e.g., WebAssembly#17) introduced new concepts necessary for the specification. This change migrates this new text to the WASI proposal template style by: - linking each section in the document index - using paragraph style more extensively - clarifying and compacting several sentences This change is not meant to introduce anything new; it should solely be a style and clarity edit.
Recent changes (e.g., #17) introduced new concepts necessary for the specification. This change migrates this new text to the WASI proposal template style by: - linking each section in the document index - using paragraph style more extensively - clarifying and compacting several sentences
As is being discussed [elsewhere], either calling `proc_exit` or trapping in any thread should halt execution of all threads. The Wasmtime CLI already has logic for adapting a WebAssembly error code to a code expected in each OS. This change factors out this logic to a new function, `maybe_exit_on_error`, for use within the `wasi-threads` implementation. This will work reasonably well for CLI users of Wasmtime + `wasi-threads`, but embedders will want something better in the future: when a `wasi-threads` threads fails, they may not want their application to exit. Handling this is tricky, because it will require cancelling the threads spawned by the `wasi-threads` implementation, something that is not trivial to do in Rust. With this change, we defer that work until later in order to provide a working implementation of `wasi-threads` for experimentation. [elsewhere]: WebAssembly/wasi-threads#17
As is being discussed [elsewhere], either calling `proc_exit` or trapping in any thread should halt execution of all threads. The Wasmtime CLI already has logic for adapting a WebAssembly error code to a code expected in each OS. This change factors out this logic to a new function, `maybe_exit_on_error`, for use within the `wasi-threads` implementation. This will work reasonably well for CLI users of Wasmtime + `wasi-threads`, but embedders will want something better in the future: when a `wasi-threads` threads fails, they may not want their application to exit. Handling this is tricky, because it will require cancelling the threads spawned by the `wasi-threads` implementation, something that is not trivial to do in Rust. With this change, we defer that work until later in order to provide a working implementation of `wasi-threads` for experimentation. [elsewhere]: WebAssembly/wasi-threads#17
This commit includes a set of changes that add initial support for `wasi-threads` to Wasmtime: * feat: remove mutability from the WasiCtx Table This patch adds interior mutability to the WasiCtx Table and the Table elements. Major pain points: * `File` only needs `RwLock<cap_std::fs::File>` to implement `File::set_fdflags()` on Windows, because of [1] * Because `File` needs a `RwLock` and `RwLock*Guard` cannot be hold across an `.await`, The `async` from `async fn num_ready_bytes(&self)` had to be removed * Because `File` needs a `RwLock` and `RwLock*Guard` cannot be dereferenced in `pollable`, the signature of `fn pollable(&self) -> Option<rustix::fd::BorrowedFd>` changed to `fn pollable(&self) -> Option<Arc<dyn AsFd + '_>>` [1] https://github.com/bytecodealliance/system-interface/blob/da238e324e752033f315f09c082ad9ce35d42696/src/fs/fd_flags.rs#L210-L217 * wasi-threads: add an initial implementation This change is a first step toward implementing `wasi-threads` in Wasmtime. We may find that it has some missing pieces, but the core functionality is there: when `wasi::thread_spawn` is called by a running WebAssembly module, a function named `wasi_thread_start` is found in the module's exports and called in a new instance. The shared memory of the original instance is reused in the new instance. This new WASI proposal is in its early stages and details are still being hashed out in the [spec] and [wasi-libc] repositories. Due to its experimental state, the `wasi-threads` functionality is hidden behind both a compile-time and runtime flag: one must build with `--features wasi-threads` but also run the Wasmtime CLI with `--wasm-features threads` and `--wasi-modules experimental-wasi-threads`. One can experiment with `wasi-threads` by running: ```console $ cargo run --features wasi-threads -- \ --wasm-features threads --wasi-modules experimental-wasi-threads \ <a threads-enabled module> ``` Threads-enabled Wasm modules are not yet easy to build. Hopefully this is resolved soon, but in the meantime see the use of `THREAD_MODEL=posix` in the [wasi-libc] repository for some clues on what is necessary. Wiggle complicates things by requiring the Wasm memory to be exported with a certain name and `wasi-threads` also expects that memory to be imported; this build-time obstacle can be overcome with the `--import-memory --export-memory` flags only available in the latest Clang tree. Due to all of this, the included tests are written directly in WAT--run these with: ```console $ cargo test --features wasi-threads -p wasmtime-cli -- cli_tests ``` [spec]: https://github.com/WebAssembly/wasi-threads [wasi-libc]: https://github.com/WebAssembly/wasi-libc This change does not protect the WASI implementations themselves from concurrent access. This is already complete in previous commits or left for future commits in certain cases (e.g., wasi-nn). * wasi-threads: factor out process exit logic As is being discussed [elsewhere], either calling `proc_exit` or trapping in any thread should halt execution of all threads. The Wasmtime CLI already has logic for adapting a WebAssembly error code to a code expected in each OS. This change factors out this logic to a new function, `maybe_exit_on_error`, for use within the `wasi-threads` implementation. This will work reasonably well for CLI users of Wasmtime + `wasi-threads`, but embedders will want something better in the future: when a `wasi-threads` threads fails, they may not want their application to exit. Handling this is tricky, because it will require cancelling the threads spawned by the `wasi-threads` implementation, something that is not trivial to do in Rust. With this change, we defer that work until later in order to provide a working implementation of `wasi-threads` for experimentation. [elsewhere]: WebAssembly/wasi-threads#17 * review: work around `fd_fdstat_set_flags` In order to make progress with wasi-threads, this change temporarily works around limitations induced by `wasi-common`'s `fd_fdstat_set_flags` to allow `&mut self` use in the implementation. Eventual resolution is tracked in #5643. This change makes several related helper functions (e.g., `set_fdflags`) take `&mut self` as well. * test: use `wait`/`notify` to improve `threads.wat` test Previously, the test simply executed in a loop for some hardcoded number of iterations. This changes uses `wait` and `notify` and atomic operations to keep track of when the spawned threads are done and join on the main thread appropriately. * various fixes and tweaks due to the PR review --------- Signed-off-by: Harald Hoyer <harald@profian.com> Co-authored-by: Harald Hoyer <harald@profian.com> Co-authored-by: Alex Crichton <alex@alexcrichton.com>
Introduce process and its exit status